-
Notifications
You must be signed in to change notification settings - Fork 8
Value less hidden properties are not allowed #187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2df804d
to
03f3e3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing a test verifying that the new behavior works. We'd need a new test pattern for this though.
"then": { | ||
"anyOf": [ | ||
{ "required": [ "value" ] }, | ||
{ "required": [ "generatedValue" ] }, | ||
{ | ||
"not": { | ||
"anyOf": [ | ||
{ "required": [ "value" ] }, | ||
{ "required": [ "generatedValue" ] } | ||
] | ||
}, | ||
"deprecated": true | ||
} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check if we can get away with a strict validation error, as suggested in #181 (comment)? What were the results? That could simplify https://github.com/camunda/element-templates-json-schema/pull/187/files#r2244641698 as we'd not need to test for warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we did check. There are alot of connectors with value-less hidden properties which results in failed integration tests.
For e.g, Amazon SQS Outbound Connector
{
"id": "configuration.endpoint",
"label": "Endpoint",
"description": "Specify endpoint if need to use custom endpoint",
"optional": true,
"group": "configuration",
"binding": {
"name": "configuration.endpoint",
"type": "zeebe:input"
},
"type": "Hidden"
},
You can check by removing this block from the properties.json
which marks value-less property as deprecated
.
{
"not": {
"anyOf": [
{ "required": [ "value" ] },
{ "required": [ "generatedValue" ] }
]
},
"deprecated": true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you let the Connectors team know about them? You can use the #ask-connectors-team channel for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bottom-line is that we rather mark the properties as deprecated as we don't want to introduce a BC.
8c8e168
to
c772a0b
Compare
@abdul99ahad This looks amazing! From what I can see:
I'd consider if/how we could use stock deprecation support in the schema (https://json-schema.org/draft/2020-12/json-schema-validation#section-9.3) over writing our own implementation. |
That's doable, the only downside is to check for warnings ,we can't integrate it with ajv validator but have to rely on recursively checking the schema after the validation to warn the user. |
I explored the stock However, using the stock approach complicates warning users. Since In contrast, our In my opinion, introducing a custom keyword like Happy to discuss alternatives or hybrid approaches if needed. |
79efc13
to
45e1140
Compare
45e1140
to
2713342
Compare
- less strict warnings
- added deprecated-warnings task
2713342
to
bbf8065
Compare
@barmac this PR needs a new owner. |
Closes #181
Proposed Changes
Value less hidden property are marked as deprecated
I’ve added the test case, but just to note: this scenario won’t trigger an error, only a deprecation warning. Currently, I’m not sure how to capture or assert against deprecation warnings through the existing validation mechanism.Test setup to catch warnings is added
Steps to try out:
You can validate the template using
element template validator
validateZeebeChecklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}